Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add explicit getters for TimelockAuthorizer #1298

Merged
merged 8 commits into from
May 20, 2022
Merged

Conversation

TomAFrench
Copy link
Contributor

This PR replaces any public state variables with private versions and a getter.

One question this raises is whether we want to be more careful about potential confusion between isPermissionGranted and hasPermission (I've added some natspec on this). One option is to not expose the isPermissionGranted getter but this would cause confusion when trying to figure out if someone's permission is local or global.

Copy link
Collaborator

@EndymionJkb EndymionJkb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like getRootTransferDelay() could be external; it's not called in the contract. And getRoot() is external (getting an immutable, so cheap when it's read directly in this contract).

pkg/vault/contracts/authorizer/TimelockAuthorizer.sol Outdated Show resolved Hide resolved
pkg/vault/contracts/authorizer/TimelockAuthorizer.sol Outdated Show resolved Hide resolved
@TomAFrench TomAFrench merged commit 4b3dc4e into master May 20, 2022
@TomAFrench TomAFrench deleted the authorizer-getters branch May 20, 2022 15:32
TomAFrench added a commit that referenced this pull request May 26, 2022
* master: (76 commits)
  Reuse swap fee code; add startWeights to weight update params for symmetry with swap fee update params (#1324)
  Ensure that global permissions are properly transferred along with root status. (#1319)
  Move up require to save gas on add token validation failure (#1318)
  Stop migrating role-admins (#1321)
  Managed Pools: Consistent LP allowlist behavior (#1322)
  Only allow root to create new granters or revokers (#1313)
  Only allow root to schedule a change to a delay (#1314)
  Switch to a two step process to migrate the root account of the Authorizer (#1316)
  Add optional key argument to verify-contract
  Add explicit getters for TimelockAuthorizer (#1298)
  Update Authorizer tests (#1299)
  fix: stop storing deployment transactions for fork test deployments (#1311)
  Automatically save the deployment tx hash on deployment (#1310)
  Update SNX impl input (#1309)
  Add implementation + deployment of the ProtocolFeesWithdrawer (#1301)
  minor update for consistency (#1306)
  fix: fix fork test for NoProtocolFeeLBP (#1304)
  ci: switch from infura to alchemy (#1305)
  Check deployment outputs (#1284)
  fix: use force argument in no fee LBP task (#1303)
  ...
dandamiangow3t pushed a commit to IlluviumGame/balancer-v2-monorepo that referenced this pull request May 10, 2023
* refactor: make root storage variable private

* refactor: make `delayerPerActionId` storage variable private

* refactor: make scheduledExecutions storage variable private

* refactor: make isPermissionGranted storage variable private

* refactor: use getRootTransferDelay getter rather than using variable directly

* refactor: rename isPermissionGranted to isPermissionGrantedOnTarget
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants